Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add generic typings for SavedObjectMigrationFn #63943

Merged
merged 13 commits into from
Apr 30, 2020

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Apr 20, 2020

Summary

  • Add generic types for input and output attributes on SavedObjectMigrationFn to allow plugin implementations to have some type checking on their migrations. Attributes types defaults to unknown
  • Adapt existing usages of SavedObjectMigrationFn to add <any, any> typings.

Checklist

  • Documentation was added for features that require explanation or tutorials

@pgayvallet pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.8.0 labels Apr 20, 2020
@pgayvallet pgayvallet requested a review from a team as a code owner April 20, 2020 08:39
@@ -39,10 +39,10 @@ import { SavedObjectsMigrationLogger } from './core/migration_logger';
*
* @public
*/
export type SavedObjectMigrationFn = (
doc: SavedObjectUnsanitizedDoc,
export type SavedObjectMigrationFn<InputProps = any, MigratedProps = any> = (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would probably require updating the types of many migrations, but is it possible to default to unknown instead of any? I think that would encourage more plugins to type their migrations and make it explicit that their migration is dangerously untyped.

What do you think about calling these generic arguments InputAttributes and MigratedAttributes since they define the attributes property?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would probably require updating the types of many migrations, but is it possible to default to unknown instead of any

I wanted to avoid impacting plugin code with that change TBH, therefor the defaulting any. But it may be better to still do it now to encourage yet-to-migrate SO migrations to be explicitly typed.

What do you think about calling these generic arguments InputAttributes and MigratedAttributes since they define the attributes property?

Indeed better. I always mix up attributes and props terminology for SOs.

@pgayvallet pgayvallet requested a review from a team April 21, 2020 07:02
@pgayvallet pgayvallet requested review from a team as code owners April 21, 2020 07:02
Comment on lines -17 to +18
export const migrations: Record<string, SavedObjectMigrationFn> = {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
export const migrations: Record<string, SavedObjectMigrationFn<any, any>> = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caused by

kibana/.eslintrc.js

Lines 736 to 741 in 40f8222

{
files: ['x-pack/plugins/lens/**/*.{ts,tsx}'],
rules: {
'@typescript-eslint/no-explicit-any': 'error',
},
},

Comment on lines +42 to +45
export type SavedObjectMigrationFn<InputAttributes = unknown, MigratedAttributes = unknown> = (
doc: SavedObjectUnsanitizedDoc<InputAttributes>,
context: SavedObjectMigrationContext
) => SavedObjectUnsanitizedDoc;
) => SavedObjectUnsanitizedDoc<MigratedAttributes>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, my only interrogation around that is that atm migrations are all mutating and returning the input doc object, causing usage of these introduced generics difficult in existing migrations.

I.E

export const migrateToKibana660: SavedObjectMigrationFn<any, any> = doc => {
  if (!doc.attributes.hasOwnProperty('disabledFeatures')) {
    doc.attributes.disabledFeatures = [];
  }
  return doc;
};

Meaning that

type InputAttrs = {
}

type OutputAttrs = {
    disabledFeatures: SomeType[];
}

export const migrateToKibana660: SavedObjectMigrationFn<InputAttrs, OutputAttrs> = doc => {
  if (!doc.attributes.hasOwnProperty('disabledFeatures')) {
    doc.attributes.disabledFeatures = [];
  }
  return doc;
};

Would fail to compile with something like property disabledFeatures does not exists on type InputAttrs on the doc.attributes.disabledFeatures = []; line.

Which is why I'm wondering if we should add the output properties on the input doc:

export type SavedObjectMigrationFn<InputAttributes = unknown, MigratedAttributes = unknown> = (
  doc: SavedObjectUnsanitizedDoc<InputAttributes & MigratedAttributes>,
  context: SavedObjectMigrationContext
) => SavedObjectUnsanitizedDoc<MigratedAttributes>;

Ideally I would have been using Partial, doc: SavedObjectUnsanitizedDoc<InputAttributes & Partial<MigratedAttributes>>, however the given example still fails with disabledFeatures is optional on type SavedObjectUnsanitizedDoc<InputAttributes & Partial<MigratedAttributes>> but required on type SavedObjectUnsanitizedDoc<MigratedAttributes> when returning the doc.

The other option being to decide that migration function should properly spread / construct the resulting migrated object without returning directly the input doc. @rudolf wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other option being to decide that migration function should properly spread / construct the resulting migrated object without returning directly the input doc.

Given the high impact of even a small cannot read property x of undefined bug in a migration I'd say it validates the bit of extra effort and memory to construct a new output object 👍

We can leave existing migrations with any as they are, but then encourage new migrations to be written in this way. We should update our documentation to show this pattern and probably add this to the developer release notes to create some more visibility so that teams don't stumble on this.

Copy link
Contributor

@rudolf rudolf Apr 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a slightly more type-safe solution than any, teams can still use InputAttributes & MigratedAttributes as their InputAttributes generic paramater.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM, I'll keep the definition as it is right now then.

Copy link
Contributor

@jportner jportner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this in mind:

We can leave existing migrations with any as they are, but then encourage new migrations to be written in this way

Security changes LGTM 👍

@rudolf
Copy link
Contributor

rudolf commented Apr 24, 2020

It causes some type errors in core, but we should remove the index signature

Done in #64434

Copy link
Contributor

@alexwizp alexwizp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM! Thank you

'7.0.0': flow<(doc: SavedObjectUnsanitizedDoc) => DashboardDoc700To720>(migrations700),
'7.3.0': flow<SavedObjectMigrationFn>(migrations730),
'6.7.2': flow<SavedObjectMigrationFn<any, any>>(migrateMatchAllQuery),
'7.0.0': flow<SavedObjectMigrationFn<any, DashboardDoc700To720['attributes']>>(migrations700),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elastic/kibana-app DashboardAttributesTo720 exists, but is not exported from bwc/types. I used DashboardDoc700To720['attributes'] instead.

Comment on lines -98 to +92
const doc700: DashboardDoc700To720 = migrations['7.0.0'](doc);
const doc700 = migrations['7.0.0'](doc, mockContext);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This migration is now an explicit SavedObjectMigrationFn so the second parameter is now needed in the signature, so I had to adapt the tests.

Copy link
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AppArch code changes look fine to me.

Only question: do we need all those any? Would it be a big ask to put in the right types, even if they don't completely define the saved object but only the part necessary for the migration?

cc @stacey-gammon as she has opinion about anys.

@streamich
Copy link
Contributor

Alternatively, maybe we can create an issue to fix those anys and tag Team:AppArch and Team:KibanaApp.

Copy link
Contributor

@rudolf rudolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy for this to be merged once we've updated the tsdocs code sample.

@@ -62,7 +62,7 @@ function migrateIndexPattern(doc: DashboardDoc700To720) {
doc.attributes.kibanaSavedObjectMeta.searchSourceJSON = JSON.stringify(searchSource);
}

const migrations700: SavedObjectMigrationFn = (doc): DashboardDoc700To720 => {
const migrations700: SavedObjectMigrationFn<any, any> = (doc): DashboardDoc700To720 => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const migrations700: SavedObjectMigrationFn<any, any> = (doc): DashboardDoc700To720 => {
const migrations700: SavedObjectMigrationFn<DashboardDoc700To720['attributes'], DashboardDoc700To720['attributes']> = doc => {

Comment on lines +114 to +116
'6.7.2': flow<SavedObjectMigrationFn<any, any>>(migrateMatchAllQuery),
'7.0.0': flow<SavedObjectMigrationFn<any, DashboardDoc700To720['attributes']>>(migrations700),
'7.3.0': flow<SavedObjectMigrationFn<any, any>>(migrations730),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'6.7.2': flow<SavedObjectMigrationFn<any, any>>(migrateMatchAllQuery),
'7.0.0': flow<SavedObjectMigrationFn<any, DashboardDoc700To720['attributes']>>(migrations700),
'7.3.0': flow<SavedObjectMigrationFn<any, any>>(migrations730),
'6.7.2': flow<typeof migrateMatchAllQuery>(migrateMatchAllQuery),
'7.0.0': flow<typeof migrations700>(migrations700),
'7.3.0': flow<SavedObjectMigrationFn<any, any>>(migrations730),

We can't follow the same pattern with migrations730 because it's not typed as a SavedObjectMigrationFn.

@@ -26,7 +26,7 @@ import { SavedObjectsMigrationLogger } from './core/migration_logger';
*
* @example
* ```typescript
* const migrateProperty: SavedObjectMigrationFn = (doc, { log }) => {
* const migrateProperty: SavedObjectMigrationFn<MyUnmigratedAttributes, MyMigratedAttributes> = (doc, { log }) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should update the example to show how to construct a copy so that the output is typed different to the input. Since this is only for type safety we don't need to make a deep copy, so it might be worth adding a comment to the example to highlight this.

@rudolf
Copy link
Contributor

rudolf commented Apr 28, 2020

@streamich This doesn't weaken any types, it only makes it explicit that some migration function have been relying on any all along. I agree that it would be good to create a follow-up issue with a task for each of the teams to update their migrations' type safety.

@streamich
Copy link
Contributor

streamich commented Apr 28, 2020

I think just one issue should be enough. The reason I suggested to tag both teams is because it appears that AppArch is codeowners of some of these changes, but those migrations might be owned KibanaApp soon instead.

@pgayvallet
Copy link
Contributor Author

I agree that it would be good to create a follow-up issue with a task for each of the teams to update their migrations' type safety.

I will create a follow-up issue with the list of files I had to add <any, any> in and ping the respective owners.

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kibana app changes LGTM, thanks for improving the types! There is room for more improvement (getting rid of the anys), but those can be addressed separately - definitely out of scope for this change. I like Rudolfs suggestions.

@pgayvallet
Copy link
Contributor Author

Created #64748

@pgayvallet
Copy link
Contributor Author

retest

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from Kibana app side.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@pgayvallet pgayvallet merged commit 6e26913 into elastic:master Apr 30, 2020
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Apr 30, 2020
* add generic typings for SavedObjectMigrationFn

* change default attributes type to unknown

* update generated doc

* adapt new calls

* update generated doc

* update migration example

* fix merge conflicts
pgayvallet added a commit that referenced this pull request Apr 30, 2020
* add generic typings for SavedObjectMigrationFn

* change default attributes type to unknown

* update generated doc

* adapt new calls

* update generated doc

* update migration example

* fix merge conflicts
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 30, 2020
* master: (42 commits)
  [Ingest] Allow aggent to send metadata compliant with ECS (elastic#64452)
  [Endpoint] Remove todos, urls to issues (elastic#64833)
  [Uptime] Remove hard coded value for monitor states histograms (elastic#64396)
  Feature/send feedback link (elastic#64845)
  [ML] Moving get filters capability to admin (elastic#64879)
  Remove edit alert button from alerts list (elastic#64643)
  [EPM] Handle constant_keyword type in KB index patterns and ES index templates (elastic#64876)
  [ML] Disable data frame anaylics clone button based on permission (elastic#64830)
  Dashboard url generator to preserve saved filters from destination dashboard (elastic#64767)
  add generic typings for SavedObjectMigrationFn (elastic#63943)
  Allow to define and update a defaultPath for applications (elastic#64498)
  [Event Log] add rel=primary to saved objects for query targets (elastic#64615)
  [Lens] Use a size of 5 for first string field in visualization (elastic#64726)
  [SIEM][Lists] Removes plugin dependencies, adds more unit tests, fixes more TypeScript types
  [Ingest] Edit datasource UI (elastic#64727)
  [Lens] Bind all time fields to the time picker (elastic#63874)
  [Lens] Use suggestion system in chart switcher for subtypes (elastic#64613)
  Improve alpha messaging (elastic#64692)
  [Ingest] Allow to enable monitoring of elastic agent (elastic#63598)
  [Metrics UI] Fix alerting when a filter query is present (elastic#64575)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants